-
Notifications
You must be signed in to change notification settings - Fork 403
Fix preview crash on re-render (BadResource error) #13967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
de69c0b to
b796ec4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This normalization is related to
Windows / Linux and Project vs Single file leads to different paths. I tried to make that works because we need to delete by key, so it is important to get the right path, but obviously this modify current behavior, and leads to failed test.
|
😞 I tried to preview quarto-web with this PR, and I still encounter some issue with sass Kv, we may have an other issue. |
|
Ok so I don't manage to reproduce, maybe it was a bad state. However, as expected, change .scss file for example does trigger re-render, but the change are not shown. Though I am really not even sure this working with current 1.8 as discussed. However, testing And I am quite convinced this is because of this normalize path key. So not good to merge yet
|
|
🤦 It is late for me and I spent much time of this, now this should really be non-normalized path for lookup. I'll test tomorrow manually the behavior |
|
So the normalizing is done correctly this time. All tests passes. I am currently checking other preview features and we're good! |
Manual Testing SummaryTested using Claude Code with Chrome DevTools MCP to verify live browser updates. Test Matrix
Detailed ResultsInput file types tested:
Resource files tested:
Stress tests:
Key FindingNo BadResource errors across 15+ render cycles in all scenarios. Pre-existing Issues Found (NOT regressions)These were verified against baseline 1.9.17 - same behavior:
|
|
Yeah all tests passes with latest change too. If this is good TS change, then we can merge and build the next prerelease ! |
cscheid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 🚀
…t each render in preview mode the project context is owned by `preview()` and cleaned up when preview exits. Calling `cleanup()` here would close shared resources (like SassCache KV handles) causing BadResource errors on subsequent re-renders (#13955).
… ensure changes are picked up in preview mode
|
Thank you. I'll clean the branch and merge |
Documents the project context lifecycle fix from #13804 that resolves intermittent preview crashes when editing notebooks and qmd files.
and add a debug log message when we do so.
The fileInformationCache was using inconsistent path formats as keys, causing cache misses during preview. On Windows, paths could be stored with forward slashes but looked up with backslashes (or vice versa). Apply normalizePath() at all cache access points: - ensureFileInformationCache() in project-shared.ts (entry point) - renderForPreview() deletion in preview.ts - populateFileInformation() and inspectDocumentConfig() in inspect.ts - cleanupNotebook() in jupyter.ts Add unit tests for fileInformationCache behavior with reusable createMockProjectContext() factory in tests/unit/project/utils.ts. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move normalization logic from call sites into the cache class itself. The class now overrides get/has/set/delete to normalize keys automatically, ensuring consistent cache behavior without requiring callers to remember to normalize paths. Also fix ensureFileInformationCache fallback to create FileInformationCacheMap (not plain Map) and add test coverage for this case. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
d1f0f14 to
979807e
Compare
When re-rendering a file during
quarto previewin a project, the preview crashes withBadResource: Bad resource IDfrom the Sass cache Deno.Kv operations.Root Cause
This is a regression from #13804 which changed how
ProjectContextis managed during preview. That PR made the project context persist across re-renders (instead of being recreated each time), butrenderForPreview()still calledrenderResult.context.cleanup()after each render.The cleanup closes shared resources like the SassCache Deno.Kv handle. On subsequent re-renders, the code returns the cached
SassCacheinstance whose KV handle was already closed, causing theBadResourceerror when callingkv.get().Fix
Remove the
cleanup()call fromrenderForPreview()- the project context is owned bypreview()and should only be cleaned up when preview exits viaonCleanup()Invalidate the
fileInformationCacheentry for the rendered file before each render - sincecleanup()previously cleared all caches (includingfileInformationCache), removing the call means stale file content would be reused. The cache entry for the specific file being rendered is now deleted so changes are picked up.Fixes #13955